Add E2E tests for Azure internal loadbalancer support, fix an issue for public IP resource deletion. #45473
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
|
Hi @karataliu. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/L
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/assign @colemickens for triage |
|
@karataliu I can understand that there would be a bug about orphaning a public IP when switching from External->Internal, but I don't understand what the tag is for... we should only ever be looking at resources that are prefixed with the service UUID... and that value doesn't change unless the Service is deleted and re-created, in which case the ServiceController is going to delete that IP config, etc, and then recreate it like new (except for the other edge case caused by ServiceController tracking these things by name, rather than UUID... but that's a separate issue). I'm confused about why we would ever be considering other resources such that we need to evaluate that tag. |
|
Thanks for the comment. A previous change has removed 'getPublicIPName' from util, and add one in azure_loadbalancer.go to figure out the potential user public IP resource: I've updated this PR accordingly:
|
|
/sig azure |
|
/retest |
|
@karataliu This LGTM. Do you plan to document this on kubernetes.github.io repo? It would be good, if you do, if you can mention that the PIP resource has to be in the same resource group as the cluster? (Unless I missed it, this all still looks to be oriented around resources "names" all in the context of a single RG.) /lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
the
do-not-merge
|
/assign @ixdy |
test/e2e/service.go
Outdated
| @@ -1241,6 +1241,77 @@ var _ = framework.KubeDescribe("Services", func() { | |||
| framework.CheckReachabilityFromPod(true, normalReachabilityTimeout, namespace, acceptPodName, svcIP) | |||
| framework.CheckReachabilityFromPod(true, normalReachabilityTimeout, namespace, dropPodName, svcIP) | |||
| }) | |||
|
|
|||
| It("should be able to create an internal type load balancer [Slow]", func() { | |||
There was a problem hiding this comment.
maybe put Azure in the name, since this is an Azure-only test?
k8s-github-robot
removed
the
lgtm
|
@ixdy update pushed. |
|
@colemickens Yes, current implementation would require the public IP resource to be in the same resource group of the cluster. I'm to prepare the doc update PR later. |
|
@colemickens Please help review: kubernetes/website#4082 |
|
@karataliu: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/approve Thanks! Can you please squash your commits, too? |
|
I don't think e2e tests need a tracking issue. |
k8s-github-robot
added
the
approved
…or public IP resource deletion.
karataliu
force-pushed
the
AzureInternalLoadBalancerE2E
branch
from
70f375f
to
f8ae27d
Compare
|
squash commit pushed. thanks. |
|
/lgtm |
k8s-ci-robot
added
lgtm
ixdy
removed
the
do-not-merge
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: colemickens, ixdy, karataliu Associated issue requirement bypassed by: ixdy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
sorry for delay; should this go into 1.7? |
|
Thanks for update. I'd suggest making this in v1.7 . The previous PR#43510 for Azure internal LoadBalancer support is new in v1.7, and this one brings e2e case and bug fix. |
|
ok, tagging it as v1.7. @kubernetes/kubernetes-release-managers @dchen1107 |
|
@karataliu is there an issue associated with the bug this is fixing? |
|
Automatic merge from submit-queue |
|
@ixdy no issue created yet, the origin feature issue is here: #38901 Shall I create one? Recap the bug description here based on fixups: For external LoadBalancer feature on Azure, if a user exposes a Kubernetes service with LoadBalancer type, a new public IP resource will be created. And when the service got deleted, the public IP resource should also be deleted. In #42034, we supported user specified IP for LoadBalancer on Azure, that is, a user could provision a public IP resource at first, and then specify it as the Currently the logic for determining whether the public IP resource should be deleted is by checking whether The key thing is how to identify a public IP resource is managed or not. One solution could be determine by the name, since the managed public IPs will have conventional naming, which includes cluster name and service uuid. |
What this PR does / why we need it:
Special notes for your reviewer:
Add new Azure resource tag to Public IP resources to indicate kubernetes managed resources.
Currently we determine whether the public IP resource should be deleted by looking at LoadBalancerIp property on spec. In the scenario 'Switching from external loadbalancer to internal loadbalancer with static IP', that value might have been updated for internal loadbalancer. So here we're to add an explicit tag for kubernetes managed resources.
Merge cleanupPublicIP logic into cleanupLoadBalancer
Release note:
NONE
CC @brendandburns @colemickens